fix(ci,jni): install Rust Android targets before JNI builds#28
Conversation
📝 WalkthroughWalkthroughThis PR adds infrastructure improvements to support building GlyphNet for Android JNI and WASM targets. Rust Android compilation targets are now installed in the CI workflow and local build script, artifact upload is configured to fail on missing JNI libraries, and the WASM crate now explicitly depends on the instant crate with wasm-bindgen feature enabled. ChangesBuild Target Installation and Dependencies
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Burst reliability (loss sweep)✅ PR gate status: PASS
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/glyphnet-wasm/Cargo.toml (1)
29-29: ⚡ Quick winUpdate on
instantdependency (version + security)
instantpinned to0.1.13is the latest crates.io release.- No known RustSec security advisories are reported for
instant0.1.13.instantis marked unmaintained (RustSec RUSTSEC-2024-0384) and recommends migrating toweb-time; consider whether you want to take that maintenance risk and/or addcargo-auditto catch future advisories.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/glyphnet-wasm/Cargo.toml` at line 29, The Cargo.toml currently pins the unmaintained dependency "instant" (instant = { version = "0.1.13", features = ["wasm-bindgen"] }) which is flagged by RUSTSEC-2024-0384; either migrate to the recommended maintained crate "web-time" (replace the Cargo.toml entry, update feature flags as needed, and update all code references from instant::Instant (or instant::now()) to web_time::Instant / web_time::now()) or explicitly document/accept the maintenance risk and add cargo-audit as a dev-dependency to detect future advisories (add cargo-audit to [dev-dependencies] and CI). Ensure you update import paths and any API differences in functions/classes that used instant so builds and tests pass.apps/expo-glyphnet/scripts/build-glyphnet-jni-android.sh (1)
14-16: 💤 Low valueConsider consistent error handling for missing build tools.
The script exits gracefully (exit 0 with WARNING) when
cargoorcargo-ndkis missing, but will fail hard ifrustup target addfails. This inconsistency may surprise developers who expect the same graceful degradation behavior.If the intent is to skip JNI builds when any build tool is unavailable or misconfigured, consider wrapping the rustup command with similar error handling. If the intent is to fail when tools are present but misconfigured, the current behavior is correct.
🔄 Optional: Add consistent error handling
echo "Ensuring Rust Android targets are installed..." -rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android +if ! rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android; then + echo "WARNING: Failed to install Rust Android targets. Skipping GlyphNet JNI build." + exit 0 +fiNote: Only apply this if graceful degradation for all missing tools is the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/expo-glyphnet/scripts/build-glyphnet-jni-android.sh` around lines 14 - 16, The rustup target installation step (rustup target add aarch64-linux-android armv7-linux-androideabi x86_64-linux-android) currently fails hard while earlier checks for cargo/cargo-ndk exit gracefully; update the script so the rustup command's failure is handled consistently — run rustup target add, capture its exit status, and if it fails echo a WARNING describing the failure (include stderr) and exit 0 to skip JNI builds (or exit nonzero if you prefer hard failure), mirroring the existing cargo/cargo-ndk error handling pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/expo-glyphnet/scripts/build-glyphnet-jni-android.sh`:
- Around line 14-16: The rustup target installation step (rustup target add
aarch64-linux-android armv7-linux-androideabi x86_64-linux-android) currently
fails hard while earlier checks for cargo/cargo-ndk exit gracefully; update the
script so the rustup command's failure is handled consistently — run rustup
target add, capture its exit status, and if it fails echo a WARNING describing
the failure (include stderr) and exit 0 to skip JNI builds (or exit nonzero if
you prefer hard failure), mirroring the existing cargo/cargo-ndk error handling
pattern.
In `@crates/glyphnet-wasm/Cargo.toml`:
- Line 29: The Cargo.toml currently pins the unmaintained dependency "instant"
(instant = { version = "0.1.13", features = ["wasm-bindgen"] }) which is flagged
by RUSTSEC-2024-0384; either migrate to the recommended maintained crate
"web-time" (replace the Cargo.toml entry, update feature flags as needed, and
update all code references from instant::Instant (or instant::now()) to
web_time::Instant / web_time::now()) or explicitly document/accept the
maintenance risk and add cargo-audit as a dev-dependency to detect future
advisories (add cargo-audit to [dev-dependencies] and CI). Ensure you update
import paths and any API differences in functions/classes that used instant so
builds and tests pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 306ae3b9-730a-4b49-80eb-716da6546c2f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/android-jni.ymlapps/expo-glyphnet/scripts/build-glyphnet-jni-android.shcrates/glyphnet-wasm/Cargo.toml
Scanner perf (ribbon + matrix fixtures)✅ PR gate status: PASS
Matrix subset
|
Summary by CodeRabbit
Chores